firmware_uefi: emit telemetry for secureboot compliance#3966
firmware_uefi: emit telemetry for secureboot compliance#3966maheeraeron wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds secure-boot compliance telemetry by comparing loaded NVRAM Secure Boot variables (PK/KEK/db/dbx) against a “base template” signature set, plumbing the base-template variables through the UEFI config path and triggering a post-injection callback to emit telemetry.
Changes:
- Add
base_secure_boot_template_varsalongsidecustom_uefi_varsacross OpenVMM config/manifest plumbing so the UEFI device can evaluate “base template presence”. - Add an
NvramStorage::after_custom_vars_injected()hook and call it on first boot after custom vars injection (when secure boot is enabled). - Implement signature-list parsing and telemetry emission in
hcl_compat_uefi_nvram_storage, plus unit tests and new deps.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm_core/vm_manifest_builder/src/lib.rs | Adds base_secure_boot_template_vars to UefiManifest::new and stores it in UefiConfig. |
| vm/devices/firmware/uefi_nvram_storage/src/lib.rs | Extends NvramStorage with after_custom_vars_injected() and forwards through trait objects. |
| vm/devices/firmware/hcl_compat_uefi_nvram_storage/src/lib.rs | Adds base-template tracking, signature parsing, and telemetry reporting for secure boot vars. |
| vm/devices/firmware/hcl_compat_uefi_nvram_storage/Cargo.toml | Adds uefi_nvram_specvars and uefi_specs dependencies. |
| vm/devices/firmware/firmware_uefi/src/service/nvram/spec_services/mod.rs | Exposes after_custom_vars_injected() passthrough on spec services. |
| vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs | Calls the new hook after injecting custom vars on first boot (secure boot enabled). |
| vm/devices/firmware/firmware_uefi/src/resolver.rs | Builds BaseSecureBootTemplateVariables from template vars and attaches them to the NVRAM backend. |
| vm/devices/firmware/firmware_uefi_resources/src/lib.rs | Adds base_secure_boot_template_vars field to UefiConfig (Protobuf). |
| petri/src/vm/openvmm/construct.rs | Plumbs base template vars separately from the final custom vars in Petri VM construction. |
| openvmm/openvmm_entry/src/ttrpc/mod.rs | Initializes new config field to default in the ttrpc path. |
| openvmm/openvmm_entry/src/lib.rs | Splits base-template vars from final custom vars and passes both into manifest/config. |
| openvmm/openvmm_defs/src/config.rs | Adds base_secure_boot_template_vars to the worker Config (MeshPayload). |
| openvmm/openvmm_core/src/worker/dispatch.rs | Plumbs new config field into internal Manifest (MeshPayload) and defaults in TODO path. |
| openhcl/underhill_core/src/worker.rs | Adds base_secure_boot_template_vars into firmware_uefi_resources::UefiConfig construction. |
| Cargo.lock | Records new dependency usage in the lockfile. |
smalis-msft
left a comment
smalis-msft
left a comment
There was a problem hiding this comment.
This feels like a pretty wide-reaching change to just add some telemetry. Is all this wiring really needed?
Not really, we're still ping-ponging back and forth with AzSec to clarify what contexts they need given that we cannot predict every customer's custom uefi json injection intent. I'm now looking to trim the plumbing, but we do need a lot of the plumbing to add in a EDIT: I am thinking we can remove plumbing of the baseline revision string and the customization mode enum. Instead, we'd rather have AzSec derive the intent from the facts openhcl can determine rather than doing work to infer |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
vm/devices/firmware/hcl_compat_uefi_nvram_storage/src/lib.rs:58
SignatureSetonly keys on(EFI_SIGNATURE_DATA, bytes)(signature owner + payload) and does not include theEFI_SIGNATURE_LIST.signature_type. That can conflate X509 and SHA256 entries if the same owner/payload bytes appear under different signature types, producing incorrect “baseline present/missing” counts. Include the signature_type GUID in the set key so entries are compared with the same semantics as the UEFI signature database.
type SignatureSet = BTreeSet<(EFI_SIGNATURE_DATA, Vec<u8>)>;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
vm/devices/firmware/hcl_compat_uefi_nvram_storage/src/lib.rs:761
- This unit test asserts that signature comparison ignores
signature_owner, but earlier review discussion for this PR indicated we should require full signature equality (including owner). If the intended semantics are owner-sensitive (to avoid false compliance), this test should be inverted (and renamed) so it fails unless the implementation includessignature_ownerin the comparison key.
fn secure_boot_template_comparison_ignores_signature_owner() {
let baseline = signature_set(&x509_variable(TEST_OWNER, &[b"cert1"]));
let loaded = signature_set(&x509_variable(Guid::new_random(), &[b"cert1"]));
assert_eq!(baseline, loaded);
| /// Revision of the Secure Boot baseline represented by the built-in templates. | ||
| /// Update this value when the built-in templates are updated to a new baseline. | ||
| /// TODO: Should we change our baseline identifier? | ||
| pub const BASELINE_REVISION: &str = "July 2026"; |
There was a problem hiding this comment.
Unsure if this is what we want to claim as our baseline revision or something else. Do we have existing names for terms or some kind of way to identify whenever we make template updates?
Per AzSec ask, emit telemetry evaluating whether a secure boot variable in NVRAM contains the base secure boot template signatures.